👋 Hello JAndrassy, we appreciate your contribution to this project!

Generated by 🚫 dangerJS against 5cb167f

@lucasssvaz looks fine to me too.

There should have been a compile-time warning whenever those now empty flush() calls are being called.
Now we have to go through all code to see what has been broken by this change (and yes it does have consequences, this change)

So for any similar changes in future PRs, please make compile time warnings whenever those are being called as it is obviously a legacy mistake when you make a call to a function which is now empty and did do stuff before.
Those warnings can be temporary until the new major release of the SDK is being released, but it would prevent lots of lost hours in searching why stuff is now no longer working like before.

For example in my project I have searched for countless hours to see why some of my boards suddenly refused to respond to new calls via the webserver and no idea what caused this as it was nearly impossible to reproduce.

@TD-er
the unread chars were in the way?
you can add the warning in your installation of the platform, until you resolve the occurrences

In my specific use case I have a wrapper for handling chunked transfers when sending replies to HTTP requests.
And this used to work very well, also when handling several quick requests in a row like the browser asking for the CSS while loading the page.
However with updating my code to comply with this change (I changed the code to use clear() instead of flush() for the webserver.client()) it is now working as it used to do.

Without my change, the device would appear to be inaccessible for quite a while to just about any HTTP request when it had to serve multiple requests in a burst.

The obvious symptoms were:

• CSS not loading
• JavaScript not loading
• JSON's were not fetched.

But my main issue with this is that it is a very significant change, which should have had some compile-time warnings about the API changes.
So for other similar changes, please consider adding such warnings as this really has taken quite a lot of time for me to search for the cause as I really had not thought of this change.

in this case it was not possible to add a warning as the flush() function is a valid function (pure virtual in the Client class) and it is not possible to know when it is used wrong

@JAndrassy
Just to let you know the utter frustration I have from this PR being merged as it keeps biting me over and over again.

in this case it was not possible to add a warning as the flush() function is a valid function (pure virtual in the Client class) and it is not possible to know when it is used wrong

This is exactly why it is a real pain in the *ss to have this PR being merged.

You simply don't know when it is being used wrong, not at compile time, not at runtime. You only keep seeing those symptoms.

In the platform packages provided by Jason for me to be able to implement support for ESP-IDF5.x this PR was reverted as it is nearly impossible to find all occurences where this is used incorrectly.
However when using 'vanilla' Arduino code I keep running into these really really annoying issues caused by this PR where you run into vague and hard to reproduce timeouts, failed connections, hanging device as clients used up all available connections, stalls, etc.

I do understand that maybe in the past there may have been a poor choice in function naming. But the way this is now 'fixed' is NOT how it should have done as it is simply breaking stuff and you can't tell where it is all broken unless spending countless hours fixing stuff and with any new lib that may be relying on the older implementation you can do it all over again.
And worst of all as I need to keep multiple versions of the SDK compilable it really messes up the code which should not even have to worry about this at all.

All those hours needlessly spent on these kind of issues cannot be spent to make sure I can get rid of those older SDK requirements or even implement nice new features.

IMHO the best way forward would be to generally revert this PR and if there really is a need to make similar changes, there should be found a way to make it work so devs can see at compile-time what needs to be changed.

why is it hard to find occurrences of "flush()" in the source code?

You just can't replace flush() with clear() in the code.
If it was as simple like that, then there wouldn't be an empty flush() function in the code, right?
There is apparently a need for both functions in some base class.

As I also need to have my code to work with SDK versions not using this PR.
So this would then result in some global define where based on the platform code you define it either as flush() or clear().
This can also be required in library code, which must then be adapted too and made sure to keep those changes when updating such a library.

So this is only enforcing the point I tried to make before as why this PR is causing a mess for developers.
You need to keep track of stuff where you should not need to worry about it.

While you're at it, can you also take a look at these:

That's exactly in the area where multiple reports have been filed lately, so I guess it might be related.

in Parsing.cpp the client.flush() is at the end of the processing of the HTTP request. the response is already sent. If the connection is not kept alive for the next request, then it is closed so why clearing it. If the connection is kept alive for the following HTTP request, then clear() could delete beginning of the following request. But characters remaining in the buffer would make the following request invalid too. So for a reused connection the processing of the request must ensure to read until the exact end of the request.

In ArduinoOTA the UDP object is used to receive and send messages. The function _onRx (invoked before that flush()) reads a message and then sends a response. So it is unclear what the flush() was meant to do. If espota.py uploads work then everything is OK. But it would fail if _onRx doesn't read the whole incoming message because then all following parsePacket will always return 0 (and this is not a good behavior of the NetworkUDP class).

One of the git blame of these 2 was about a commit of 6+ years ago where the last commit text mentioned something about it being added intentionally. (some commit inbetween about code formatting)
I'm on an extremely slow laptop right now, so I can't get these git blames to load here.

Regardless if it might be some code smell of a problem elsewhere, it does seem like those 2 had to be changed too when this PR was merged to keep at least the functionality the same.
Do you agree those two occurences should be changed to .clear() too?
If so, then I will make a PR (or you can do one) later this evening.

Still the best solution would to revert this PR completely. The regressions caused by this PR are hughe. Even two follow up PRs are already needed to fix the core itself! How many will be needed at end just for the core itself?
The situation is worse with the many many libs out there. The are all incompatible now.
Please @me-no-dev revert this PR!!

How about instead of reverting it, we have flush() to call clear()? Would that help you all?

I think that's a nice compromise.
Maybe also add some deprecated attribute if possible, so whenever it is used the compiler will warn?

PR with clear and deprecation created: #10242

@me-no-dev Good solution, no silent breaking change anymore. Now there is an info that a mandatory change has be done and still all code is working as expected.